feat: add some Smctr CSRs #1172
Conversation
sudo-apt-abdullah
requested review from
ThinkOpenly and
dhower-qc
as code owners
ThinkOpenly
left a comment
ThinkOpenly
left a comment
There was a problem hiding this comment.
Pretty good!
Could you add support for this from the spec?
All fields are optional except for M, S, U, and BPFRZ. All unimplemented fields are read-only 0, while all implemented fields are writable. If the Sscofpmf extension is implemented, LCOFIFRZ must be writable.
|
Do you expect to include the rest of the Smctr CSRs?
...or shall we punt that to a later PR? |
|
The CI tests are failing due to dependence on #554. (FYI) |
Yes, I’ll add that.
I’d prefer to refine these first and get them merged before moving on to the rest — maybe open another PR for the remaining ones. |
|
@sudo-apt-abdullah, the changes in #554 have been merged (via #1238). Would you be willing to rebase this PR? Then we can do a proper review and work to get it merged. |
ThinkOpenly
left a comment
ThinkOpenly
left a comment
There was a problem hiding this comment.
@sudo-apt-abdullah, let us know whether you are willing to continue with this PR, or if we should reassign. There's still a bit of work to do, and I don't want to presume. :-)
| Bits<12> mctrctl_addr = 0x34E; | ||
| Bits<64> mctrctl_mask = 0x787F9F80000FC7; | ||
| Csr csr_handle = direct_csr_lookup(mctrctl_addr); | ||
| if (!csr_handle.valid) { | ||
| unimplemented_csr($encoding); | ||
| } |
There was a problem hiding this comment.
Given that this file defines the CSR, we don't need to check if it's implemented here.
| Bits<12> mctrctl_addr = 0x34E; | |
| Bits<64> mctrctl_mask = 0x787F9F80000FC7; | |
| Csr csr_handle = direct_csr_lookup(mctrctl_addr); | |
| if (!csr_handle.valid) { | |
| unimplemented_csr($encoding); | |
| } | |
| Bits<64> mctrctl_mask = 0x787F9F80000FC7; |
We'll need to work on that mask, too, to avoid magic numbers. Here's what I suggest:
| Bits<12> mctrctl_addr = 0x34E; | |
| Bits<64> mctrctl_mask = 0x787F9F80000FC7; | |
| Csr csr_handle = direct_csr_lookup(mctrctl_addr); | |
| if (!csr_handle.valid) { | |
| unimplemented_csr($encoding); | |
| } | |
| Bits<64> mctrctl_mask = 0; | |
| if (MCTRCTL_RASEMU_IMPLEMENTED) { | |
| mctrctl_mask = mctrctl_mask | (1 << 6); | |
| } |
... for each field/bit.
We still have that magic number "6", but maybe that's OK for now. :-/
That "IMPLEMENTED" test, above, is because the CTR spec says:
All fields are optional except for M, S, U, and BPFRZ.
So, each optional field/bit needs to be tested and will depend on creating a new parameter. Those will look like something like this (in spec/std/isa/ext/Smctr.yaml):
params:
MCTRCTL_RASEMU_IMPLEMENTED:
description: |
Indicates whether or not the `RASEMU` field is imiplemented in `mctrctl` CSR.
schema:
type: boolean
We'll also need type() attributes for each bit field which makes them read-only-0 depending on whether each is IMPLEMENTED.
2 participants
Closes #555